test(e2e): add nominal tests for exec & init CLI commands #434

ErwanRaulo · 2024-12-31T17:54:56Z

TOFIX: could run tests before TS migration, now something goes wrong with childprocess and execution path.

when running nreport init, main process throws the same error :
Error: ENOENT: no such file or directory, open 'C:<yourDevDir>\report\dist\views\template.html'

process try to access views from dist folder but it does not exists, should be added to dist folder as asset in tsconfig file to include a copy when compiling typescript

ErwanRaulo · 2024-12-31T18:46:28Z

package.json

@@ -19,6 +19,7 @@
    "lint": "eslint src test bin scripts",
    "test-only": "glob -c \"tsx --test-reporter=spec --test\" \"./test/**/*.spec.ts\"",
    "test": "c8 --all --src ./src -r html npm run test-only",
+    "test:e2e": "glob -c \"tsx --test-reporter=spec --test\" \"./test/commands/*.spec.ts\"",


i think functional tests are not executed during development as we expect quick feedback from small unit tests.

It's fine to have a separate script for e2e tests, however, the glob from existing test-only script still match e2e tests files.
I think it's simpler to handle this via file names, for instance:

{ "test-only": "glob -c \"tsx --test-reporter=spec --test\" \"./test/**/*.spec.ts\"", "test:e2e": "glob -c \"tsx --test-reporter=spec --test\" \"./test/**/*.e2e-spec.ts\"" }

ErwanRaulo · 2025-01-01T00:23:24Z

test/helpers/reportCommandRunner.ts

+}
+
+export function filterProcessStdout(options, filter): Promise<string[]> {
+  return new Promise((resolve, reject) => {


childprocess was turned into promise in order to collect filtered stdout outputs and emit them all at the end.

fraxken · 2025-01-03T12:36:02Z

test/commands/execute.spec.ts

+    let actualLines: string[] = [];
+
+    try {
+      actualLines = await filterProcessStdout(options, byMessage);
+    }
+    catch (error) {
+      console.log(error);
+
+      assert.fail("Execute command should not throw");
+    }
+
+    assert.deepEqual(actualLines, expectedLines, "we are expecting these lines");


Suggested change

let actualLines: string[] = [];

try {

actualLines = await filterProcessStdout(options, byMessage);

}

catch (error) {

console.log(error);

assert.fail("Execute command should not throw");

}

assert.deepEqual(actualLines, expectedLines, "we are expecting these lines");

const actualLines = await filterProcessStdout(options, byMessage);

assert.deepEqual(actualLines, expectedLines, "we are expecting these lines");

Pas forcément besoin de rendre complexe juste pour un message de failure spécifique

fraxken · 2025-01-03T12:38:14Z

test/commands/initialize.spec.ts

+const __dirname = path.dirname(fileURLToPath(import.meta.url));
+const processDir = path.join(__dirname, "..", "fixtures");
+const configFilePath = path.join(processDir, ".nodesecurerc");
+
+describe("Report init command", async() => {
+  beforeEach(async() => await fs.unlink(configFilePath));


Il faudrait plutôt créer la configuration dans le path temp pour ne pas avoir de problèmes avec une modification non-prévu du fichier dans /fixtures

PierreDemailly · 2025-01-17T14:42:24Z

Hi @ErwanRaulo

when running nreport init, main process throws the same error :
Error: ENOENT: no such file or directory, open 'C:\report\dist\views\template.html'

I don't have this problem, are you sure you built the project first ?
The "rimraf dist/views && cp -r views dist/views" script should work 🤔

no breaking changes use childprocess to collect CLI stdout missing limit case tests fix NodeSecure#395

PierreDemailly · 2025-01-20T21:59:17Z

package.json

@@ -19,6 +19,7 @@
    "lint": "eslint src test bin scripts",
    "test-only": "glob -c \"tsx --test-reporter=spec --test\" \"./test/**/*.spec.ts\"",
    "test": "c8 --all --src ./src -r html npm run test-only",
+    "test:e2e": "glob -c \"tsx --test-reporter=spec --test\" \"./test/commands/*.spec.ts\"",


It's fine to have a separate script for e2e tests, however, the glob from existing test-only script still match e2e tests files.
I think it's simpler to handle this via file names, for instance:

{ "test-only": "glob -c \"tsx --test-reporter=spec --test\" \"./test/**/*.spec.ts\"", "test:e2e": "glob -c \"tsx --test-reporter=spec --test\" \"./test/**/*.e2e-spec.ts\"" }

PierreDemailly · 2025-01-20T22:01:41Z

test/commands/execute.spec.ts

+import dotenv from "dotenv";
+dotenv.config();


Suggested change

import dotenv from "dotenv";

dotenv.config();

Require dotenv in the script instead ? This way we don't need to import it in multiple files
"test:e2e": "glob -c \"tsx -r dotenv/config --test-reporter=spec --test\" \"./test/commands/*.spec.ts\""

PierreDemailly

Nit picks, this is good job 💪

test/commands/execute.spec.ts

PierreDemailly · 2025-01-20T22:04:44Z

test/commands/execute.spec.ts

+const __dirname = path.dirname(fileURLToPath(import.meta.url));
+const processDir = path.join(__dirname, "../..");
+
+describe("Report execute command", async() => {


Suggested change

describe("Report execute command", async() => {

describe("Report execute command", () => {

PierreDemailly · 2025-01-20T22:06:56Z

test/commands/execute.spec.ts

+    }
+
+    const expectedLines = [
+      "Executing nreport at: C:\\PERSO\\dev\\report",


Suggested change

"Executing nreport at: C:\\PERSO\\dev\\report",

`Executing nreport at: ${kProcessDir}`,

PierreDemailly · 2025-01-20T22:07:17Z

test/commands/initialize.spec.ts

+import dotenv from "dotenv";
+dotenv.config();
+


Suggested change

import dotenv from "dotenv";

dotenv.config();

Same as above

PierreDemailly · 2025-01-20T22:07:34Z

test/commands/initialize.spec.ts

+
+// CONSTANTS
+const __dirname = path.dirname(fileURLToPath(import.meta.url));
+const processDir = path.join(__dirname, "..", "fixtures");


Suggested change

const processDir = path.join(__dirname, "..", "fixtures");

const kProcessDir = path.join(__dirname, "..", "fixtures");

I wonder why it's prefixed by k ?

This is a convention for all the constants in all our projects: file-scoped constants should be prefixed with k, exported constants should be named using CONSTANT_CASE/UPPER_CASE, and other constants should be named using camelCase :)

AFAIK this convention comes from the Google C++ style guide used in Node.js (it's more used for Symbols tho)
Examples:
https://github.com/nodejs/node/blob/main/lib/events.js#L62
https://github.com/nodejs/node/blob/main/lib/events.js#L87
Another example at NodeSecure:
https://github.com/NodeSecure/cli/blob/master/src/http-server/cache.js#L13

PierreDemailly · 2025-01-20T22:07:41Z

test/commands/initialize.spec.ts

+// CONSTANTS
+const __dirname = path.dirname(fileURLToPath(import.meta.url));
+const processDir = path.join(__dirname, "..", "fixtures");
+const configFilePath = path.join(processDir, ".nodesecurerc");


Suggested change

const configFilePath = path.join(processDir, ".nodesecurerc");

const kConfigFilePath = path.join(processDir, ".nodesecurerc");

PierreDemailly · 2025-01-20T22:08:08Z

test/commands/initialize.spec.ts

+const processDir = path.join(__dirname, "..", "fixtures");
+const configFilePath = path.join(processDir, ".nodesecurerc");
+
+describe("Report init command", async() => {


Suggested change

describe("Report init command", async() => {

describe("Report init command", () => {

ErwanRaulo · 2025-01-20T22:22:54Z

Thanks for feedback, I will correct them all soon.

And yes I can run " nreport init " now, don't known if it was thanks to the latest commit or my fault but in my TS migration PR I've used rimraf also.

ErwanRaulo force-pushed the test-cli-integration branch 2 times, most recently from 9425a2f to bae3766 Compare December 31, 2024 23:46

ErwanRaulo commented Jan 1, 2025

View reviewed changes

fraxken reviewed Jan 3, 2025

View reviewed changes

ErwanRaulo force-pushed the test-cli-integration branch from bae3766 to 1c7450a Compare January 19, 2025 17:24

test(e2e): add nominal tests for exec & init CLI commands

1c7450a

no breaking changes use childprocess to collect CLI stdout missing limit case tests fix NodeSecure#395

PierreDemailly reviewed Jan 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(e2e): add nominal tests for exec & init CLI commands #434

test(e2e): add nominal tests for exec & init CLI commands #434

ErwanRaulo commented Dec 31, 2024 •

edited

Loading

ErwanRaulo Dec 31, 2024

PierreDemailly Jan 20, 2025

ErwanRaulo Jan 1, 2025

fraxken Jan 3, 2025

fraxken Jan 3, 2025

fraxken Jan 3, 2025

PierreDemailly commented Jan 17, 2025

PierreDemailly Jan 20, 2025

PierreDemailly Jan 20, 2025

PierreDemailly left a comment

PierreDemailly Jan 20, 2025

PierreDemailly Jan 20, 2025

PierreDemailly Jan 20, 2025

PierreDemailly Jan 20, 2025

ErwanRaulo Jan 20, 2025

PierreDemailly Jan 20, 2025

PierreDemailly Jan 20, 2025

PierreDemailly Jan 20, 2025

ErwanRaulo commented Jan 20, 2025

	describe("Report execute command", async() => {
	describe("Report execute command", () => {

	"Executing nreport at: C:\\PERSO\\dev\\report",
	`Executing nreport at: ${kProcessDir}`,

	const processDir = path.join(__dirname, "..", "fixtures");
	const kProcessDir = path.join(__dirname, "..", "fixtures");

	const configFilePath = path.join(processDir, ".nodesecurerc");
	const kConfigFilePath = path.join(processDir, ".nodesecurerc");

	describe("Report init command", async() => {
	describe("Report init command", () => {

test(e2e): add nominal tests for exec & init CLI commands #434

Are you sure you want to change the base?

test(e2e): add nominal tests for exec & init CLI commands #434

Conversation

ErwanRaulo commented Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PierreDemailly commented Jan 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PierreDemailly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErwanRaulo commented Jan 20, 2025

ErwanRaulo commented Dec 31, 2024 •

edited

Loading